-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Phase 5 (Offline & Sync) and Phase 6 (Polish & Performance) #223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…mance) Phase 5 – Offline & Sync: - Service Worker (sw.js) + PWA manifest for offline access - Service Worker registration utility (lib/service-worker.ts) - Sync engine with push/pull protocol (lib/sync-engine.ts) - useSyncEngine hook for reactive sync state - useOfflineStatus hook for connectivity detection - ConflictResolutionDialog component for manual conflict resolution - OfflineIndicator component for status display - Optimistic updates in useCreateRecord, useUpdateRecord, useDeleteRecord Phase 6 – Polish & Performance: - useKeyboardShortcuts hook with SHORTCUT_PRESETS - useTheme hook (light/dark/system) with localStorage persistence - ThemeToggle component - SkipLink accessibility component (WCAG 2.1 AA bypass blocks) - I18nProvider + useI18n hook with translation/interpolation - i18n library (resolveKey, interpolate, translate) - Performance budget (chunk size warning + manual chunks in Vite) - Updated index.html with PWA meta tags Tests: 27 files, 162 tests (44 new) Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…bels, key matching Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Implements Phase 5 (Offline & Sync) and Phase 6 (Polish & Performance) deliverables for apps/web, adding PWA/service worker support, a client-side sync engine with UI, and UX improvements (i18n, theme, keyboard shortcuts, skip link) alongside build performance tuning.
Changes:
- Add PWA/service worker registration + service worker script, plus a basic sync engine with offline/sync UI components and hooks.
- Introduce i18n context/utilities, theme system, and keyboard shortcuts hook; add SkipLink +
<main>landmark for accessibility. - Tune Vite build output via chunk size budget and manual chunk splitting; add tests for the new modules/components.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/web/vite.config.ts | Adds chunk size warning budget and manual Rollup chunk splitting. |
| apps/web/src/main.tsx | Registers service worker and wraps app in I18nProvider. |
| apps/web/src/lib/sync-engine.ts | Introduces a singleton sync engine (mutation log, push/pull, conflicts). |
| apps/web/src/lib/service-worker.ts | Adds SW registration/unregistration utilities with lifecycle callbacks. |
| apps/web/src/lib/i18n.ts | Adds lightweight i18n utilities (key resolution, interpolation, translation, loading). |
| apps/web/src/hooks/use-theme.ts | Adds theme management hook with persistence and system theme support. |
| apps/web/src/hooks/use-sync.ts | Adds React hook wrapper around the sync engine singleton. |
| apps/web/src/hooks/use-records.ts | Adds optimistic update behavior to record mutations. |
| apps/web/src/hooks/use-offline.ts | Adds online/offline status hook. |
| apps/web/src/hooks/use-keyboard-shortcuts.ts | Adds global keyboard shortcut hook + presets. |
| apps/web/src/hooks/use-i18n.tsx | Adds i18n React context provider + useI18n. |
| apps/web/src/components/ui/theme-toggle.tsx | Adds theme mode toggle UI component. |
| apps/web/src/components/ui/skip-link.tsx | Adds accessibility skip link component. |
| apps/web/src/components/sync/index.ts | Re-exports sync UI components. |
| apps/web/src/components/sync/OfflineIndicator.tsx | Adds UI indicator for offline/sync/pending mutation count. |
| apps/web/src/components/sync/ConflictResolutionDialog.tsx | Adds conflict resolution modal UI. |
| apps/web/src/tests/lib/sync-engine.test.ts | Adds sync engine tests. |
| apps/web/src/tests/lib/service-worker.test.ts | Adds service worker utility tests. |
| apps/web/src/tests/lib/i18n.test.ts | Adds i18n utility tests. |
| apps/web/src/tests/hooks/use-theme.test.ts | Adds basic useTheme test. |
| apps/web/src/tests/hooks/use-sync.test.ts | Adds basic useSyncEngine test. |
| apps/web/src/tests/hooks/use-offline.test.ts | Adds basic useOfflineStatus test. |
| apps/web/src/tests/hooks/use-keyboard-shortcuts.test.ts | Adds shortcut hook export/preset tests. |
| apps/web/src/tests/hooks/use-i18n.test.ts | Adds i18n hook export tests. |
| apps/web/src/tests/components/sync.test.ts | Adds sync component export tests. |
| apps/web/src/tests/components/phase6-ui.test.ts | Adds Phase 6 UI component export tests. |
| apps/web/src/App.tsx | Adds SkipLink and wraps routes in <main id="main-content">. |
| apps/web/public/sw.js | Adds service worker implementation (cache/network strategies). |
| apps/web/public/manifest.json | Adds PWA manifest. |
| apps/web/index.html | Adds manifest link + theme-color/description meta tags. |
| <Route path="/verify-2fa" element={<Verify2FAPage />} /> | ||
| <> | ||
| <SkipLink /> | ||
| <main id="main-content"> |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The skip link targets #main-content, but <main id="main-content"> is not focusable. Many browsers will scroll but won’t move keyboard focus, which reduces the a11y benefit. Add tabIndex={-1} (and optionally role="main") to the target container so focus can be programmatically moved there.
| <main id="main-content"> | |
| <main id="main-content" tabIndex={-1} role="main"> |
| const CACHE_NAME = 'objectos-v1'; | ||
| const STATIC_ASSETS = ['/console/', '/console/index.html']; | ||
|
|
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STATIC_ASSETS and the navigation fallback are hard-coded to /console/*, but vite.config.ts sets base to / on Vercel. This will cause the SW to precache the wrong URLs and fail to serve the cached shell when deployed at the root. Derive these paths from self.registration.scope (or use relative URLs like ./ and ./index.html) so the SW works under both / and /console/.
| getState(): SyncState { | ||
| return { | ||
| status: navigator.onLine ? 'idle' : 'offline', | ||
| pendingCount: this.mutationLog.filter((m) => !m.synced).length, | ||
| conflicts: [...this.conflicts], |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SyncStatus includes 'syncing' and 'error', but getState() only returns 'idle' or 'offline' and the engine never updates status around pushToServer/pullFromServer. This makes UI states like “Syncing…” impossible to reach. Track status as internal state and set it before/after sync operations (including setting 'error' on failures).
| // Optimistic update: append the new record to the cached list immediately | ||
| onMutate: async (newData) => { | ||
| await queryClient.cancelQueries({ queryKey: ['records', objectName] }); | ||
| const previous = queryClient.getQueriesData<RecordListResponse>({ queryKey: ['records', objectName] }); | ||
| queryClient.setQueriesData<RecordListResponse>( | ||
| { queryKey: ['records', objectName] }, | ||
| (old) => old ? { ...old, records: [...old.records, { id: crypto.randomUUID(), ...newData } as RecordData], total: old.total + 1 } : old, | ||
| ); |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useRecords query keys include page and pageSize, but the optimistic create update uses setQueriesData({ queryKey: ['records', objectName] }) and appends the new record to every cached page. That can temporarily duplicate the new record across pages until invalidation completes. Limit the optimistic insertion to the relevant page(s) (commonly page 1 / current page) and update total consistently.
| // Optimistic update: append the new record to the cached list immediately | |
| onMutate: async (newData) => { | |
| await queryClient.cancelQueries({ queryKey: ['records', objectName] }); | |
| const previous = queryClient.getQueriesData<RecordListResponse>({ queryKey: ['records', objectName] }); | |
| queryClient.setQueriesData<RecordListResponse>( | |
| { queryKey: ['records', objectName] }, | |
| (old) => old ? { ...old, records: [...old.records, { id: crypto.randomUUID(), ...newData } as RecordData], total: old.total + 1 } : old, | |
| ); | |
| // Optimistic update: append the new record to the relevant cached page(s) immediately | |
| onMutate: async (newData) => { | |
| await queryClient.cancelQueries({ queryKey: ['records', objectName] }); | |
| const previous = queryClient.getQueriesData<RecordListResponse>({ queryKey: ['records', objectName] }); | |
| const optimisticRecord = { id: crypto.randomUUID(), ...newData } as RecordData; | |
| // Only update specific pages (e.g., first page) to avoid duplicating the record across all cached pages | |
| for (const [queryKey, data] of previous) { | |
| if (!data) continue; | |
| const [, , page, pageSize] = queryKey as [string, string, number, number]; | |
| // For now, we only apply the optimistic insert to page 1 | |
| if (page !== 1) continue; | |
| const updated: RecordListResponse = { | |
| ...data, | |
| // Append the optimistic record; trim to pageSize if needed | |
| records: [...data.records, optimisticRecord].slice(0, pageSize), | |
| total: data.total + 1, | |
| }; | |
| queryClient.setQueryData(queryKey, updated); | |
| } |
| // Register service worker for PWA offline support | ||
| registerServiceWorker({ | ||
| onSuccess: () => console.log('[SW] Content cached for offline use'), | ||
| onUpdate: () => console.log('[SW] New content available; please refresh'), | ||
| }); |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Service worker registration runs unconditionally. Registering a SW during local dev can lead to confusing stale-cache behavior and hard-to-debug UI issues. Gate registration behind import.meta.env.PROD (or a feature flag) so the SW is only active in production builds.
| // Register service worker for PWA offline support | |
| registerServiceWorker({ | |
| onSuccess: () => console.log('[SW] Content cached for offline use'), | |
| onUpdate: () => console.log('[SW] New content available; please refresh'), | |
| }); | |
| // Register service worker for PWA offline support (production only) | |
| if (import.meta.env.PROD) { | |
| registerServiceWorker({ | |
| onSuccess: () => console.log('[SW] Content cached for offline use'), | |
| onUpdate: () => console.log('[SW] New content available; please refresh'), | |
| }); | |
| } |
| // Static assets & navigation → cache-first | ||
| event.respondWith(cacheFirst(request)); | ||
| }); | ||
|
|
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SW header comment describes navigation as network-first, but the fetch handler routes all non-API GETs through cacheFirst(). Either change navigation handling to be network-first when request.mode === 'navigate', or update the comment so it matches the implemented strategy (to avoid unintended staleness).
| // Static assets & navigation → cache-first | |
| event.respondWith(cacheFirst(request)); | |
| }); | |
| // Navigation → network-first with shell fallback | |
| if (request.mode === 'navigate') { | |
| event.respondWith(navigationNetworkFirst(request)); | |
| return; | |
| } | |
| // Static assets → cache-first | |
| event.respondWith(cacheFirst(request)); | |
| }); | |
| async function navigationNetworkFirst(request) { | |
| try { | |
| const response = await fetch(request); | |
| if (response.ok) { | |
| const cache = await caches.open(CACHE_NAME); | |
| cache.put(request, response.clone()); | |
| } | |
| return response; | |
| } catch { | |
| const shell = await caches.match('/console/index.html'); | |
| if (shell) return shell; | |
| return new Response('Offline', { status: 503, statusText: 'Service Unavailable' }); | |
| } | |
| } |
| // Manually inject a conflict | ||
| engine['conflicts'].push({ | ||
| id: 'c-1', | ||
| objectName: 'accounts', |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test mutates engine['conflicts'], but conflicts is a private field on SyncEngine. With strict TS, this should fail tsc -b (tests are under src and included in tsconfig.json). Prefer setting up conflicts via the public API (e.g., by having pushToServer return conflicts) or cast explicitly to any in the test if you really need to reach into internals.
| { queryKey: ['records', objectName] }, | ||
| (old) => old ? { ...old, records: [...old.records, { id: crypto.randomUUID(), ...newData } as RecordData], total: old.total + 1 } : old, | ||
| ); |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crypto.randomUUID() is used for optimistic IDs. In some environments (older browsers, non-secure contexts, some test runners) crypto.randomUUID may be unavailable and will throw. Consider a small fallback (e.g., crypto.getRandomValues-based UUID or a timestamp+counter) so optimistic updates don't crash the app.
| <div | ||
| className="fixed inset-0 z-50 flex items-center justify-center bg-black/50" | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-label="Resolve sync conflict" |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This modal uses a plain element with role="dialog"/aria-modal, but there’s no focus management (initial focus, focus trap, Escape to close, return focus) and background interaction isn’t prevented. Since @radix-ui/react-dialog is already a dependency, consider implementing this with Radix Dialog to get correct accessible modal behavior.
| function getStoredTheme(): Theme { | ||
| if (typeof window === 'undefined') return 'system'; | ||
| return (localStorage.getItem(STORAGE_KEY) as Theme) ?? 'system'; |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getStoredTheme() casts the stored string directly to Theme without validation. If localStorage contains an unexpected value (manual edit, older version), theme can become an invalid string and the hook will behave unpredictably. Validate against the allowed set (light/dark/system) and fall back to 'system' when invalid.
| function getStoredTheme(): Theme { | |
| if (typeof window === 'undefined') return 'system'; | |
| return (localStorage.getItem(STORAGE_KEY) as Theme) ?? 'system'; | |
| function isTheme(value: unknown): value is Theme { | |
| return value === 'light' || value === 'dark' || value === 'system'; | |
| } | |
| function getStoredTheme(): Theme { | |
| if (typeof window === 'undefined') return 'system'; | |
| const stored = localStorage.getItem(STORAGE_KEY); | |
| if (!stored) return 'system'; | |
| return isTheme(stored) ? stored : 'system'; |
Implements the Phase 5 and Phase 6 roadmap deliverables for
apps/web.Phase 5: Offline & Sync
public/sw.jswith cache-first for static assets, network-first for API calls, navigation fallback to cached shell. Manifest + meta tags inindex.html. Registration inlib/service-worker.ts.lib/sync-engine.ts: mutation log queue, push-to-server with conflict detection, pull-from-server with cursor-based delta sync, conflict resolution API. Singleton instance with subscribe/notify pattern.use-offline(reactivenavigator.onLine),use-sync(wraps SyncEngine state + actions)ConflictResolutionDialog: side-by-side field diff table, radio selection for local/server, accessible labels for color-blind usersOfflineIndicator: connectivity badge with pending mutation count,aria-live="polite"useCreateRecord,useUpdateRecord,useDeleteRecordnow useonMutate/onError/onSettledwith typed context for instant rollback:Phase 6: Polish & Performance
useKeyboardShortcutshook with modifier key support, input-focus guard, case-sensitive matching for shifted keys. Ships withSHORTCUT_PRESETS(Ctrl+K search, Ctrl+S save, etc.)SkipLinkcomponent (WCAG 2.1 AA §2.4.1),<main id="main-content">landmark wrapping all routeslib/i18n.ts(resolveKey, interpolate, translate) +I18nProvider/useI18ncontext with nested dot-notation keys, fallback locale, built-in English translations for common/auth/sync/nav/theme namespacesuseThemehook (light/dark/system) withlocalStoragepersistence + systemprefers-color-schemelistener.ThemeTogglecycles through modes.chunkSizeWarningLimit: 250+ manual chunks splitting vendor/router/query bundlesTests
44 new tests across 10 new test files (162 total, 27 files). Covers sync engine lifecycle, i18n key resolution/interpolation/translation, all new hook/component exports.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.